-
Notifications
You must be signed in to change notification settings - Fork 153
improv(tests): adopt new aws cdk cli library #1624
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Kudos, SonarCloud Quality Gate passed!
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR, Andrea! Finally the testing resources got their dedicated place outside of the commons package.
I see this PR as a great improvement step to the previous structure and usage. I still have few concerns I'd like to address, which might require a bigger refactoring that do not belong here.
Name overload
We overload resource names in many places, i.e. NodeJsFunction
is our own wrapper, plus the actual CDK class, plus the factory that creates the CDK NodeJsFunction. Over time it will make our life harder to reason and distinguish about them, so we need a good separation in the name between our internal resources and the CDK classes.
Resource zoo
I notices that we currently support few resources, like DynamoDB table, NodeJsFunction and SSMParameters. They are all sitting in one class, wired through multiple if-statements so we know how to construct the TestCase. The list of resources will grow and with 10-20 different resources it'd require new restructure. There are multiple places in the code base that we need to change if would want to add support for SQS queue. In such cases it is often challenging to draw the line when to refactor, is it 5 or 8 or 10 resources that are "too much". My proposal would be to do it rather sooner than later, not in this PR but in the next iteration of the tests package. I don't have a particular design in mind, the driving force is primarily "what is the best setup so it'd be easier to add support for X in the future".
CDK integ lib
The examples you have highlighted in the PR description reminded me of the integ-test-alpha
package of the CDK project. While our testing code was written before the cdk testing package existed, it seems like our implementation might converge into the same direction and it wouldn't be a surprise if we build a very similar assertion helper. Thus, we need to review cdk-integ-test-alpha package and see if and how much of the package we can use that'd make our life easier. It's a trade-off of course to reduce the code base for the cost of being squared into the library capabilities, and I think it's worth reviewing (again not here, in a separate PR).
import type { IStringParameter, StringParameter } from 'aws-cdk-lib/aws-ssm'; | ||
import { PolicyStatement, Effect } from 'aws-cdk-lib/aws-iam'; | ||
|
||
class NodeJsFunction { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name overload can be confusing, I see this class is internal, so it won't be exposed for maintainers when they write tests. Still, I'd propose to distinguish between our resource wrapper and the actual resource we hold.
} | ||
|
||
public get envVariableName(): string { | ||
return this.#envVariableName ?? 'TABLE_NAME'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move this check to the constructor?
* | ||
* @default - No output | ||
*/ | ||
logGroupOutputKey?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth to also add the retention period with a sensible default, maybe a week or 5 days. What do you think?
Hey Alex, thanks for the review - I agree with the points you've made and I think it's important to get this right (or at least better than it is at the moment) now. I'll move back the PR to draft status and try to address some the comments. |
After discussing this further, me & @am29d have decided that we are going to take this back to the drawing board and consider working on a RFC before moving onto implementation. If anyone stumbles into this & would like to contribute please get in touch. |
Description of your changes
This PR introduces a new package to the workspace dedicated to testing utilities. Historically the
commons
package has been a catch-all package for all shared utilities and code that didn't belong to any specific package. This has caused the package to sprawl (#1354) and while this hasn't always had an impact on what gets published to npm in thecommons
utility, the repository could use some reorganization.In the case of integration/e2e tests specifically, there's a lot of room for improvement (#1314) since one of the most critical pieces of our testing infrastructure currently relies on an internal API from CDK that could (and has) change(d) without notice.
As a part of the new internal & unpublished
testing
utility, the PR introduces a new class calledTestStack
that relies on the@aws-cdk/cli-lib-alpha
module, which allows to interact with the AWS CDK CLI programmatically. This class incorporates many of the lessons learned and aims at streamlining the boilerplate code needed to setup the infrastructure for a test, while standardizing the resources deployed as part of it.Technical deep dive
To articulate the benefits of the new
TestStack
class, let's try to partially refactor themakeHandlerIdempotent
tests. This test suite is fairly recent and presents many of the issues and sprawl that the new class aims to solve.At a high level, our integration tests can be divided (like many tests of this type) in three discrete areas:
While this PR focuses on the first and last phases, and specifically on the "Setup" one, the new class could address some of the optimizations in the "Testing" phase in future PRs (see "Future improvements" section below).
In order to setup the architecture needed for the tests, this specific one as well as all others, all have a version of the code below:
As you can see, the code above is fairly verbose and contains a lot of boilerplate. Additionally, there's another
createIdempotencyResources
function referenced. This function is in charge of adding aNodejsFunction
and aTable
CDK constructs to the stack for each test and most of the utilities (Tracer, Logger, Metrics, Idempotency, etc.) define a version of this function to deploy resources.Once all the resources have been added to the CDK stack, in the
beforeAll
phase of the test we actually deploy the stack:Now, let's refactor the code above using the new
TestStack
class that is introduced in this PR:The benefit of this new utility are:
Future improvements
As mentioned, at the moment we are improving only the setup phase. In future PRs we could continue centralizing other parts of the integration tests like the invocation of the Lambda function in a test case, or the retrieval of CloudWatch logs for that test.
Give that each test case is now an object, we could add methods like:
or:
Related issues, RFCs
Issue number: #1623
Checklist
Breaking change checklist
Is it a breaking change?: NO
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.